-
-
Notifications
You must be signed in to change notification settings - Fork 33.1k
gh-139940: Handle RuntimeError when attaching to a non-existing process in pdb.py #139941
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
I think attaching to a non-exist process is a common case where we should produce better error messages. I have a feeling that the message should be even more explicit about "you have the wrong process id". @pablogsal and @godlygeek what do you think? |
Sounds reasonable to me. Maybe we want to catch the RuntimeError on POSIX and handle it by attempting |
Lib/pdb.py
Outdated
except RuntimeError as e: | ||
while e.__context__ is not None: | ||
e = e.__context__ | ||
print(f"Error attaching to process: {e}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to get e
here? We have opts.pid
here to inform the user. I don't think the user understands this. I think the proper way here is to figure out what are the possibilities for RuntimeError
. We should just let the user know that this failed, and the highest possibility is that the process does not exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this is just borrowed from
Lines 239 to 248 in 419441a
def _get_awaited_by_tasks(pid: int) -> list: | |
try: | |
return get_all_awaited_by(pid) | |
except RuntimeError as e: | |
while e.__context__ is not None: | |
e = e.__context__ | |
print(f"Error retrieving tasks: {e}") | |
sys.exit(1) | |
except PermissionError as e: | |
exit_with_permission_help_text() |
The idea here is to hide the traceback as it may contain 3+ frames that users don't know how to handle, and at the same time, don't arbitrarily cover up any fatal errors. And only the top-most frame is the real cause.
But I am okay to change to a better way.
Lib/pdb.py
Outdated
parser.error("argument -m: not allowed with argument --pid") | ||
try: | ||
attach(opts.pid, opts.commands) | ||
except RuntimeError as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
except RuntimeError as e: | |
except RuntimeError as exc: |
Signed-off-by: Frost Ming <me@frostming.com>
Signed-off-by: Frost Ming <me@frostming.com>
e4cf4cc
to
c50b200
Compare
Before change:
After change:
Close gh-139940